[client] Android - Reuse the profile's account for Android SSO logins - #6988
Open
pappz wants to merge 2 commits into
Open
[client] Android - Reuse the profile's account for Android SSO logins#6988pappz wants to merge 2 commits into
pappz wants to merge 2 commits into
Conversation
The Android binding never recorded which account a profile belongs to, so every interactive login and every session extend went to the IdP with no login_hint. With nothing to go on the IdP picks an account itself, which on a session extend means re-authenticating an account the profile is already signed in with. Store the email the PKCE flow already parses out of the ID token, and pass it back as the hint on later flows. An empty hint stays meaningful: a fresh profile, or one that was logged out, deliberately leaves the choice to the IdP, which is how a profile changes accounts. Logout clears the stored email for that reason — while it is on disk it would steer the next login straight back into the account just logged out of. The email is keyed off the profile's config path rather than the active profile: Auth.login runs in a goroutine, so the active profile can change under a flow already in flight. It lands in <profile>.account.json, not the <profile>.state.json desktop uses for the same data — there the email and the engine's state manager sit in different directories, but on Android both resolve under files/, and the state manager rewrites the whole file from its own keys.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAndroid clients now retain each profile’s configuration path and account email, expose email and active status in profile metadata, clear stored email on logout, and reuse the email as an SSO login hint during login and session extension. ChangesAndroid profile authentication
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Auth
participant OAuth
participant ProfileState
Client->>Auth: pass cfgPath to NewAuthWithConfig
Auth->>ProfileState: readProfileEmail(cfgPath)
ProfileState-->>Auth: stored account email
Auth->>OAuth: set login hint
OAuth-->>Auth: JWT token and account email
Auth->>ProfileState: writeProfileEmail(cfgPath, email)
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
pappz
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/android/profile_manager.go`:
- Around line 203-206: Update LogoutProfile’s removeProfileEmail cleanup path so
a failure is returned to the caller instead of being only logged and followed by
a successful nil return. Preserve the existing warning context, and ensure the
logout operation reports the cleanup error after key removal so callers can
retry and avoid stale login hints.
In `@client/android/session.go`:
- Around line 296-300: Update extendAuthSession around foregroundGetTokenInfo to
persist tokenInfo.Email only after engine.ExtendAuthSession succeeds. Reuse the
rejected-token protection established in Auth.login/client/android/login.go, and
ensure the selected email is stored for fresh, logged-out, or switched profiles
without persisting rejected tokens.
- Around line 296-300: Update ExtendAuthSession to obtain cfg and cfgPath from
one atomic stateSnapshot instead of calling configPathSnapshot separately; pass
the snapshot’s cfgPath value to NewAuthWithConfig so both configuration and
login hint data come from the same profile state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 26d59291-a456-4c0c-9681-dcd206d238dd
📒 Files selected for processing (6)
client/android/client.goclient/android/login.goclient/android/profile_manager.goclient/android/profile_state.goclient/android/profile_state_test.goclient/android/session.go
Release artifactsBuilt for PR head
GHCR images (amd64)
This comment is updated by the Release workflow. Artifact links expire according to the workflow retention policy. |
extendAuthSession took the config from stateSnapshot and the config path from a second call, each acquiring the lock on its own. A profile switch landing between the two swaps every field, which would authenticate with one profile's config while reading the login hint from another profile's account file. Replace configPathSnapshot with authSnapshot, which returns both from a single critical section.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The Android binding never recorded which account a profile belongs to, so every interactive login and every session extend went to the IdP with no login_hint. With nothing to go on the IdP picks an account itself, which on a session extend means re-authenticating an account the profile is already signed in with.
Store the email the PKCE flow already parses out of the ID token, and pass it back as the hint on later flows. An empty hint stays meaningful: a fresh profile, or one that was logged out, deliberately leaves the choice to the IdP, which is how a profile changes accounts. Logout clears the stored email for that reason — while it is on disk it would steer the next login straight back into the account just logged out of.
The email is keyed off the profile's config path rather than the active profile: Auth.login runs in a goroutine, so the active profile can change under a flow already in flight. It lands in .account.json, not the .state.json desktop uses for the same data — there the email and the engine's state manager sit in different directories, but on Android both resolve under files/, and the state manager rewrites the whole file from its own keys.
Describe your changes
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
New Features
Bug Fixes
Tests